-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Infer regions for opaque types in borrowck #67681
Infer regions for opaque types in borrowck #67681
Conversation
c3363af
to
784008b
Compare
This comment has been minimized.
This comment has been minimized.
784008b
to
2624f6f
Compare
2624f6f
to
e23a0cb
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit e23a0cb45307a173427b9fcaefe648530e5f27c6 with merge 109224fdba977e11e6046e91fe34f9430fd0203a... |
☀️ Try build successful - checks-azure |
Queued 109224fdba977e11e6046e91fe34f9430fd0203a with parent e0239b4, future comparison URL. |
Finished benchmarking try commit 109224fdba977e11e6046e91fe34f9430fd0203a, comparison URL. |
Perf is slightly green. That wasn't exactly expected, I'll investigate tomorrow. |
src/librustc/query/mod.rs
Outdated
desc { |tcx| "borrow-checking `{}`", tcx.def_path_str(key) } | ||
cache_on_disk_if(tcx, _) { key.is_local() && tcx.is_closure(key) } | ||
cache_on_disk_if { key.is_local() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you caching non-closure keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's no longer just closures that can have interesting borrow checking results. I didn't realize that this could depend on the query's result though, I can change it to that if that wouldn't cause problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can look at the values, yes. What's looking at non-closure borrow checking results now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type_of
for opaque types is looking at it.
e23a0cb
to
41b92f3
Compare
}; | ||
|
||
if !opaque_type_values.is_empty() { | ||
err.note(&format!("Inferred opaque type values:\n{:#?}", opaque_type_values)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we want to have tests which use this.
☔ The latest upstream changes (presumably #67886) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I did a first pass. Everything looks like it makes sense but I left two comments for what seem to be the most "load-bearing" parts of the PR that I want to re-read. Thanks for pushing on this, @matthewjasper!
// Map back to "concrete" regions so that errors in | ||
// `infer_opaque_definition_from_instantiation` can show | ||
// sensible region names. | ||
let universal_concrete_type = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a hopefully helpful comment to this function.
Some(opaque_defn_ty) => opaque_defn_ty, | ||
}; | ||
debug!("opaque_defn_ty = {:?}", opaque_defn_ty); | ||
let subst_opaque_defn_ty = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should read this again.
b840ce6
to
ed6cb3f
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #67901) made this pull request unmergeable. Please resolve the merge conflicts. |
3069435
to
f2cebdc
Compare
I've made some more changes that may affect performance |
* Use better span for member constraint errors * Avoid a bad suggestion * Don't report member constraint errors if we have other universal region errors.
Also correctly calculate what the upper bounds are.
This ensures that NLL will infer suitable values for regions in opaque types when it's possible.
93e6ed7
to
d863978
Compare
@bors r=nikomatsakis |
📌 Commit d863978 has been approved by |
…omatsakis Infer regions for opaque types in borrowck This is a step towards the goal of typeck not doing region inference. The commits up to `Arena allocate the result of mir_borrowck` are various bug fixes and prerequisites. The remaining commits move opaque type inference to borrow checking. r? @nikomatsakis
☀️ Test successful - checks-azure |
👌🏼 |
This is a step towards the goal of typeck not doing region inference.
The commits up to
Arena allocate the result of mir_borrowck
are various bug fixes and prerequisites.The remaining commits move opaque type inference to borrow checking.
r? @nikomatsakis